Support OpenMP #145
Support OpenMP #145
Conversation
|
@eddelbuettel now you know why I have so so many organizations 😉 |
| fi | ||
|
|
||
| ## Now use all these | ||
| AC_SUBST([OPENMP_FLAG], ["${openmp_flag}"]) |
There was a problem hiding this comment.
Just so I understand, what purpose does OPENMP_FLAG serve given that you already accumulate in PKG_CXXFLAGS and PKG_LIBS options for linking libomp?
There was a problem hiding this comment.
As I recall one (or two) of the three macOS variants needs it / sets it as relying on SHLIB_OPENMP_CXXFLAGS is not sufficient there and we need PKG_{CXXFLAGS,LIBS} there. However, in the simpler (Linux) case we do not catch these and set this one as a simpler fallback right before the use you quote:
# Overall summary
AC_MSG_CHECKING([for OpenMP])
if test x"${can_use_openmp}" = x"yes"; then
AC_MSG_RESULT([found and suitable])
openmp_flag='$(SHLIB_OPENMP_CXXFLAGS)'
else
AC_MSG_RESULT([missing so no OpenMP acceleration])
openmp_flag=""
fiThe pattern I borrow here from myself in RcppArmadillo has taken a few weeks to mature and now seems to work reliably so I thought it was beneficial to use it here too. The configure.ac is way more complicated that I would have wanted -- on Linux I simply place SHLIB_OPENMP_CXXFLAGS in there unconditionally and R takes care of it -- but macOS appears to require special love and care we now give it.
There was a problem hiding this comment.
Yes, I was thinking about accidental duplication of options in the concatenation of SHLIB_OPENMP_CXXFLAGS and PKG_CXXFLAGS, but I see now that it's precluded thanks to the initial "works out of the box" test.
There was a problem hiding this comment.
Pull request overview
This PR adds OpenMP support to RcppEigen to enable multi-threaded parallel computations within the Eigen library. The implementation follows the approach from RcppArmadillo with robust support for Linux/Windows and enhanced macOS detection.
Key changes:
- Adds autoconf-based OpenMP detection with platform-specific fallbacks
- Implements thread control functions and automatic core throttling respecting R options and CRAN limits
- Updates documentation to describe threading behavior and control functions
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| configure.ac | Adds autoconf script for OpenMP detection across Linux, macOS, and Windows |
| configure | Generated configure script from autoconf |
| src/Makevars.in | Template for Makevars with OpenMP flag substitution |
| src/Makevars.win | Windows Makefile updated to use SHLIB_OPENMP_CXXFLAGS |
| src/Makevars | Removed static Makevars (now generated from .in template) |
| src/RcppEigen.cpp | Adds EigenSetNbThreads function and refactors eigen_version |
| src/RcppExports.cpp | Exports new thread control functions |
| R/init.R | New startup code to set initial thread count based on Ncpus and OMP_THREAD_LIMIT |
| R/RcppExports.R | Exports EigenSetNbThreads and eigen_version_typed |
| NAMESPACE | Exports new thread control functions |
| man/RcppEigen_throttle_cores.Rd | Documents thread control API |
| man/RcppEigen-package.Rd | Updates package documentation with threading information |
| cleanup | Adds config.log and config.status to cleanup targets |
| DESCRIPTION | Adds RoxygenNote field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coatless No luck though. I don't think it ever built that branch of that added repo / entry in packages.json. Hmpf. |
|
I pivoted over at R universe and had this built as 0.4.9.8-0 and it is looking good. Screenshot to preserver it: 
So I may merge this tomorrow, rebase and have the feature/eigen-5.0.0 branch be built there again as 0.4.9.9-0. |
Closes #144
See the discussion in #144 and the motivating examples. This branch (and PR) follows RcppArmadillo in i) relying on the robust support under Linux and Windows where we can trust the
$(SHLIB_OPENMP_CXXFLAGS)variable and ii) fairly refined and extensive additional testing for the various ways in which macOS may or may not have OpenMP support which, which arguably overly complicated, has been seen to work there. Our use case is actually even a little simpler: All OpenMP uses happen inside of Eigen, none are in oursrc/directory apart from passing the desired number of cores on.We follow the same logic: if either option
Ncpusis set (as is common via other R packages) or ifOMP_THREAD_LIMITis set (as is the case for CRAN) then it is respected. Otherwise we let Eigen use what it sees, which may be all cores.This change should lead to a net speedup in the majority of use cases on all three operating systems.
/cc @coatless as I also just borrowed his
coatless-rpkgorg to try a second alternate r-universe build (given that my own has the pending Eigen 5.0.0 PR build)